Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: use course enrollment instead of course mode to calculate audit trial is expired #146

Merged
merged 1 commit into from
Jan 6, 2025

Conversation

varshamenon4
Copy link
Member

COSMO-616

As I was trying to understand how to set the upgrade deadline (i.e. the topic in standup about prioritizing audit vs. honor), I ran into some questions around CourseEnrollment and CourseMode which might change how audit_trial_is_expired is implemented.

Context

Currently audit_trial_is_expired gets the CourseMode (this is where it gets multiple) using the courserun_key and then uses the expiration_datetime (aka upgrade_deadline) to calculate whether the audit trial is expired

But, the CourseMode expiration_datetime corresponds to the expiration of this course mode. So if "For example, if there is a verified mode that expires on 1/1/2015, then users will be able to upgrade into the verified mode before that date. Once the date passes, users will no longer be able to enroll as verified." This means that for the purpose of the audit_trial_is_expired, we want to get the upgrade deadline for the verified mode, not for the audit or honor mode which is what we would be doing now, since the learner is an audit learner

Proposed Solution

I propose that we use the CourseEnrollment upgrade_deadline to get the upgrade deadline for the course.

This method gets the verified modes (if there are any) and calculates the upgrade deadline, which is what we need for this method.

learning_assistant/api.py Outdated Show resolved Hide resolved
learning_assistant/api.py Outdated Show resolved Hide resolved
@varshamenon4 varshamenon4 force-pushed the varshamenon4/fix-multiple-course-mode-issue branch from 9a02d9c to 24ba4e6 Compare December 18, 2024 16:28
@varshamenon4 varshamenon4 changed the title fix: change course mode to course enrollment fix: use course enrollment instead of course mode to calculate audit trial is expired Dec 18, 2024
tests/test_api.py Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Dec 18, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  learning_assistant
  __init__.py
  api.py
  views.py
Project Total  

This report was generated by python-coverage-comment-action

@varshamenon4 varshamenon4 force-pushed the varshamenon4/fix-multiple-course-mode-issue branch 3 times, most recently from 7e3b2e7 to 9507a0a Compare December 19, 2024 19:32
Copy link
Member Author

@varshamenon4 varshamenon4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added a comment explaining why and what has changed in the audit trial calculation.

* audit_trial_is_expired (boolean): whether the audit trial is expired
"""
upgrade_deadline = enrollment.upgrade_deadline
today = date.today()

# If the upgrade deadline has passed, return True for expired. Upgrade deadline is an optional attribute of a
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I've changed the way that this is calculated to avoid issues with timezone naive/aware timezones. This now converts all of the datetimes into dates so that we're just calculating the difference in whole days (I believe this is more consistent with the calculations of days remaining in the frontend as well).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where in the JavaScript we're treating an expiration date as a whole day. I'm looking at the useCourseUpgrade hook. The Date represents the date as milliseconds since the epoch, so the comparison is done on the order of milliseconds and not days. I can see that we're computing the number of days left by dividing by the number of milliseconds in a day and taking the ceiling, but the actual computation of the number of time left is on the order of milliseconds.

Can we resolve the timezone issues by constructing the datetime the same way (i.e. timezone-aware or timezone-naive datetimes) between the two places that have issues - sounds like the code and the tests?

Can you help me understand why we're calculating this by date? My concern is that we're using time data in the calculation on the frontend but not on the backend. Wouldn't a learner's audit trials be considered expired at midnight on the last day of the trial?

The impact of this is that the frontend determines that the trial is not-expired, but the new Python code considers it expired. This means that the chat is going to show for the learner on the frontend, but the backend is going to return a 403 when the learner sends a message.

Here's an example I worked through.

Python
This shows that removing the time component results in two different values between the old and new code for calculating if the upgrade deadline has passed and if the expiration date has passed.

// Set up.
>>> from datetime import date, datetime, timedelta
>>> today_with_time = today_with_time = datetime(2024, 12, 19, 15, 29, 37, 901850)
>>> today_with_time
datetime.datetime(2024, 12, 19, 15, 29, 37, 901850)
>>> today_without_time = date(2024, 12, 19)
>>> upgrade_deadline = datetime(2024, 12, 19, 16, 29, 37, 901850) // 1 hour from now

// Old way of computing whether a trial is expired, with time.
// Note that the return value is False.
>>> days_until_upgrade_with_time = today_with_time - upgrade_deadline
>>> days_until_upgrade_with_time
datetime.timedelta(days=-1, seconds=82800)
>>> past_deadline_with_time = days_until_upgrade_with_time >= timedelta(days=0)
>>> past_deadline_with_time
False

// New way of computing whether a trial is expired, without time.
// Note that the return value is True.
>>> days_until_upgrade_without_time = today_without_time - upgrade_deadline.date()
>>> past_deadline_without_time = days_until_upgrade_without_time.days >= 0
>>> past_deadline_without_time
True

// I know the variable name is upgrade_deadline here, but just imagine it's audit_trial_data.expiration_date instead.
expiration_date = upgrade_deadline
>>> expiration_date <= today_with_time
False
>>> expiration_date.date() <= today_without_time
True

JavaScript

// Set up.
let auditTrialExpirationDate = new Date('2024-12-19T16:29:37.901850');
const millisecondsInOneDay = 24 * 60 * 60 * 1000;

auditTrialExpirationDate
Thu Dec 19 2024 16:29:37 GMT-0500 (Eastern Standard Time)

// To simulate the expiration date being one hour from now, use a variable instead of Date.now().
now = new Date('2024-12-19T15:29:37.901850');

now
Thu Dec 19 2024 15:29:37 GMT-0500 (Eastern Standard Time)

// The number of audit trial days remaining is 1. I substituted Date.now() with now.
let auditTrialDaysRemaining = Math.ceil((auditTrialExpirationDate - now) / millisecondsInOneDay);

auditTrialDaysRemaining
1

// The audit trial is not yet expired.
let auditTrialExpired = auditTrialDaysRemaining < 0

auditTrialExpired
false

Copy link
Member

@rijuma rijuma Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think forcing the timezone as tz=None might cause issues, since the dates should be treated in the same timezone.

In the case of the frontend, the date received is in UTC but when a date is created like in the hook as new Date(auditTrial.expirationDate), the resulting date is converted to the local timezone, that's why it can be compared with Date.now() since in the frontend, both dates are in the same timezone.

I'm no expert on Python, but I think it should be similar, so parsing a UTC time should (IMHO) be converted to the server timezone, so it should be able to be compared with datetime.now().

I don't think truncating the dates would be a viable solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call! I've updated to have the timezone set when creating datetime.now() to have it be timezone aware so it can be compared with the other times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know anything about the upgrade_deadline in terms of the timezone? Is it always in UTC? Or is it in the learner's local server time? Is it an aware datetime? I assume so given the need to keep today aware.

I'm concerned about the use of timezone.utc here, because we may be comparing against another timezone in upgrade_deadline, and Python isn't going to automatically convert today to the learner's local server time or to the same timezone that upgrade_deadline is in.

Copy link
Member Author

@varshamenon4 varshamenon4 Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do some more research on this but the reason I did this was because my understanding is that when comparing two aware datetimes, the timezone is considered. So you can compare two aware datetimes and the timezone is taken into account. See here: https://stackabuse.com/comparing-datetimes-in-python-with-and-without-timezones/.

If it's better I could convert all the timezones to UTC but I didn't think this was necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know that. Good call.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right that Python compares them correctly under-the-hood. I just couldn't find that state explicitly in the documentation. Looks good to me!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yes, I confirmed by testing this:

@freeze_time('2024-01-01 00:00:01')
    def test_timezone(self):
        today_utc = datetime.now(tz=timezone.utc)
        london = datetime.now(zoneinfo.ZoneInfo("Europe/London"))
        nyc = datetime.now(zoneinfo.ZoneInfo("America/New_York"))
        logging.info(today_utc)
        logging.info(london)
        logging.info(nyc)

        self.assertEqual(today_utc, london)
        self.assertNotEqual(today_utc, nyc)
        self.assertNotEqual(london, nyc)

And these are the outputs of each:

FakeDatetime(2024, 1, 1, 0, 0, 1, tzinfo=datetime.timezone.utc)
FakeDatetime(2024, 1, 1, 0, 0, 1, tzinfo=zoneinfo.ZoneInfo(key='Europe/London'))
FakeDatetime(2023, 12, 31, 19, 0, 1, tzinfo=zoneinfo.ZoneInfo(key='America/New_York'))

So it looks like the comparison of datetimes takes the timezones into account!

Copy link
Member

@MichaelRoytman MichaelRoytman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but I have some concerns about the time calculations. I tried to be very thorough about my thoughts because I personally find time confusing. Let me know what you think!

The CourseEnrollment upgrade date was a really nice find! 🎉

* audit_trial_is_expired (boolean): whether the audit trial is expired
"""
upgrade_deadline = enrollment.upgrade_deadline
today = date.today()

# If the upgrade deadline has passed, return True for expired. Upgrade deadline is an optional attribute of a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see where in the JavaScript we're treating an expiration date as a whole day. I'm looking at the useCourseUpgrade hook. The Date represents the date as milliseconds since the epoch, so the comparison is done on the order of milliseconds and not days. I can see that we're computing the number of days left by dividing by the number of milliseconds in a day and taking the ceiling, but the actual computation of the number of time left is on the order of milliseconds.

Can we resolve the timezone issues by constructing the datetime the same way (i.e. timezone-aware or timezone-naive datetimes) between the two places that have issues - sounds like the code and the tests?

Can you help me understand why we're calculating this by date? My concern is that we're using time data in the calculation on the frontend but not on the backend. Wouldn't a learner's audit trials be considered expired at midnight on the last day of the trial?

The impact of this is that the frontend determines that the trial is not-expired, but the new Python code considers it expired. This means that the chat is going to show for the learner on the frontend, but the backend is going to return a 403 when the learner sends a message.

Here's an example I worked through.

Python
This shows that removing the time component results in two different values between the old and new code for calculating if the upgrade deadline has passed and if the expiration date has passed.

// Set up.
>>> from datetime import date, datetime, timedelta
>>> today_with_time = today_with_time = datetime(2024, 12, 19, 15, 29, 37, 901850)
>>> today_with_time
datetime.datetime(2024, 12, 19, 15, 29, 37, 901850)
>>> today_without_time = date(2024, 12, 19)
>>> upgrade_deadline = datetime(2024, 12, 19, 16, 29, 37, 901850) // 1 hour from now

// Old way of computing whether a trial is expired, with time.
// Note that the return value is False.
>>> days_until_upgrade_with_time = today_with_time - upgrade_deadline
>>> days_until_upgrade_with_time
datetime.timedelta(days=-1, seconds=82800)
>>> past_deadline_with_time = days_until_upgrade_with_time >= timedelta(days=0)
>>> past_deadline_with_time
False

// New way of computing whether a trial is expired, without time.
// Note that the return value is True.
>>> days_until_upgrade_without_time = today_without_time - upgrade_deadline.date()
>>> past_deadline_without_time = days_until_upgrade_without_time.days >= 0
>>> past_deadline_without_time
True

// I know the variable name is upgrade_deadline here, but just imagine it's audit_trial_data.expiration_date instead.
expiration_date = upgrade_deadline
>>> expiration_date <= today_with_time
False
>>> expiration_date.date() <= today_without_time
True

JavaScript

// Set up.
let auditTrialExpirationDate = new Date('2024-12-19T16:29:37.901850');
const millisecondsInOneDay = 24 * 60 * 60 * 1000;

auditTrialExpirationDate
Thu Dec 19 2024 16:29:37 GMT-0500 (Eastern Standard Time)

// To simulate the expiration date being one hour from now, use a variable instead of Date.now().
now = new Date('2024-12-19T15:29:37.901850');

now
Thu Dec 19 2024 15:29:37 GMT-0500 (Eastern Standard Time)

// The number of audit trial days remaining is 1. I substituted Date.now() with now.
let auditTrialDaysRemaining = Math.ceil((auditTrialExpirationDate - now) / millisecondsInOneDay);

auditTrialDaysRemaining
1

// The audit trial is not yet expired.
let auditTrialExpired = auditTrialDaysRemaining < 0

auditTrialExpired
false

Returns:
* audit_trial_is_expired (boolean): whether the audit trial is expired
"""
upgrade_deadline = enrollment.upgrade_deadline
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! 🎉

@varshamenon4 varshamenon4 force-pushed the varshamenon4/fix-multiple-course-mode-issue branch 3 times, most recently from d84126c to 7236f79 Compare January 3, 2025 21:56
Copy link
Member

@schenedx schenedx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@varshamenon4 varshamenon4 force-pushed the varshamenon4/fix-multiple-course-mode-issue branch from 4d322fb to 0cfe0b4 Compare January 6, 2025 18:03
@varshamenon4 varshamenon4 merged commit 1e87b89 into main Jan 6, 2025
4 checks passed
@varshamenon4 varshamenon4 deleted the varshamenon4/fix-multiple-course-mode-issue branch January 6, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants